-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Set "get:schema/" API endpoint as deprecated #3813
Set "get:schema/" API endpoint as deprecated #3813
Conversation
Looks like this is problematic as we have methods in the SDK that's using the incorrect URL. For now we can perhaps instead do something like this: diff --git a/backend/infrahub/api/schema.py b/backend/infrahub/api/schema.py
index 281cac8ba..cac117343 100644
--- a/backend/infrahub/api/schema.py
+++ b/backend/infrahub/api/schema.py
@@ -135,7 +135,7 @@ def evaluate_candidate_schemas(
@router.get("")
-@router.get("/")
+@router.get("/", include_in_schema=False, deprecated=True) Then we need to change some methods in the SDK such as schema.fetch(), this part should be done first then we can remove the router get in a future version instead once the SDK package have been updated and the new versions are in use. |
I can see here infrahub/python_sdk/infrahub_sdk/schema.py Line 690 in 4c3ac41
f"{self.client.address}/api/schema/?{query_params}" . Wondering if the correct/standard way shouldn't be without the "/" i.e /api/schema?{query_params}
|
Yes that would be the correct approach. As you might note we probably have this duplicated in both the sync and async functions. |
I'll maybe create a separated PR / Issue to fix all usage we have across the SDK, any opinion on that? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, though I'd remove the part in the description that it fixes the issue (to avoid having the issue be automatically closed). Also perhaps you can change the title of the PR to reflect that the work took a different path. The titles are currently used for the automated part of the release notes (though this might change soon)
I set the get "schema/" API endpoint as deprecated and flagged it so it's not included in the schema anymore. Also, it appears that this endpoint is consumed at various places in the SDK (This will be addressed in a separated PR).
#1363